Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Clean up interfaceLockMap entries on endpoint deletion #1249

Merged
merged 14 commits into from
Feb 5, 2025

Conversation

byte-msft
Copy link
Contributor

Description

The packetParser was creating entries in interfaceLockMap for each new interface
but failing to remove them when interfaces were deleted. In environments with
high pod counts and frequent churn, this caused a memory leak as the map grew
indefinitely.

Related Issue

Potential memory leak in packetparser's interfaceLockMap #1236

Checklist

  • I have read the contributing documentation.
  • I signed and signed-off the commits (git commit -S -s ...). See this documentation on signing commits.
  • I have correctly attributed the author(s) of the code.
  • I have tested the changes locally.
  • I have followed the project's style guidelines.
  • I have updated the documentation, if necessary.
  • I have added tests, if applicable.

Screenshots (if applicable) or Testing Completed

Please add any relevant screenshots or GIFs to showcase the changes made.

Additional Notes

Solution

  • Added cleanup of interfaceLockMap entries in the EndpointDeleted case
  • Improved mutex handling logic to prevent resource leaks
  • Updated test cases to verify proper cleanup of both tcMap and interfaceLockMap

Testing

  • Added comprehensive test coverage for interface deletion scenario
  • Verified cleanup of both maps in test cases
  • Tested with high pod churn scenarios

Impact

This fix prevents memory leaks in environments with frequent pod creation/deletion,
improving the overall stability and resource usage of the system.

Please refer to the CONTRIBUTING.md file for more information on how to contribute to this project.

@byte-msft byte-msft self-assigned this Jan 21, 2025
@byte-msft byte-msft requested a review from a team as a code owner January 21, 2025 10:52
@byte-msft byte-msft linked an issue Jan 21, 2025 that may be closed by this pull request
@byte-msft byte-msft requested a review from nddq January 21, 2025 11:03
Copy link
Member

@SRodi SRodi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but I'll leave @nddq or @rbtr to review & approve

@nddq
Copy link
Contributor

nddq commented Jan 21, 2025

we should take this chance to examine whether or not do we even need these sync.Mutex for each interface, since we are processing them sequentially and not concurrently anyway

@byte-msft
Copy link
Contributor Author

we should take this chance to examine whether or not do we even need these sync.Mutex for each interface, since we are processing them sequentially and not concurrently anyway

Yeah, you make a valid point! Since we're processing interfaces sequentially, we can safely remove the mutex mechanism. Let's keep it simple and avoid unnecessary complexity. I've updated the PR to remove the mutex-related code.

Copy link
Contributor

@nddq nddq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved, although I'm still not thoroughly convinced that we need the lock map, I'll not die on this hill for this PR, will let @anubhabMajumdar sign this off

@anubhabMajumdar
Copy link
Contributor

approved, although I'm still not thoroughly convinced that we need the lock map, I'll not die on this hill for this PR, will let @anubhabMajumdar sign this off

Had a conversation offline to resolve this. Change looks good to me.

@byte-msft byte-msft added this pull request to the merge queue Feb 5, 2025
Merged via the queue into microsoft:main with commit d1378d1 Feb 5, 2025
28 checks passed
@byte-msft byte-msft deleted the fix/issues/1236 branch February 5, 2025 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Potential memory leak in packetparser's interfaceLockMap
7 participants